Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable initiator name based ACL support #44

Closed
wants to merge 1 commit into from
Closed

Enable initiator name based ACL support #44

wants to merge 1 commit into from

Conversation

mikechristie
Copy link
Contributor

This just adds support for initiator name based ACL support.
It is enabled by default and turned off if CHAP is setup.

This just adds support for initiator name based ACL support.
It is enabled by default and turned off if CHAP is setup.
Copy link

@dillaman dillaman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mikechristie
Copy link
Contributor Author

Bah, I just remembered why we didn't't do this. It assumes the user is going to do all initiator ACLs or all CHAP auth. You can't mix and match as one method disables the other.

Going to close this for now and think about if it is possible to modify the kernel to allow a mix and match under the same tpg. We could just create multiple TPGs, but that gets messy when we have to replicate and manage that.

@dillaman
Copy link

dillaman commented Jan 6, 2018

Could add a “auth chap | nochap” action right under the target to globally enable / disable CHAP and the have a “chap [password]” action under the initiators if chap is enabled.

@mikechristie
Copy link
Contributor Author

We can do what you propose at the gui/gwcli level. The problem is that the kernel will only allow you to have all chap or all non chap for all entries under a target's tpg. So we can't do something like this:

  o- hosts ...................................................... [Hosts: 2]
    o- iqn.1994-05.com.redhat:ini1 .............. [Auth: CHAP, Disks: 0(0b)]
    o- iqn.1994-05.com.redhat:ini2w .. [LOGGED-IN, Auth: None, Disks: 0(0b)]

If we did

  1. generate_node_acls=0 and authentication=0 then it will do initiator name ACLs.
  2. generate_node_acls=0 and authentication=1 then each initiator has to have CHAP setup.
  3. generate_node_acls=1 then we will use the tpg level CHAP values if authentication=1.

We would need to add something like this to the kernel:

  1. generate_node_acls=0 and authentication=0 and also a initiator level authentication setting where each initiator would also have a authentication setting that can override the tpg level one. If initiator authentication=1 we will check both the initiator name and do chap. If initiator authentication=0 then we just do the initiator name all like before.

@mikechristie
Copy link
Contributor Author

generate_node_acls=1 then we will use the tpg level CHAP values if authentication=1.

Oh yeah, that means all initiators under the target tpg would use the same username/password.

@dillaman
Copy link

dillaman commented Jan 6, 2018

@mikechristie Yeah, I was thinking all or none on the full target as a starting point to cover cases where folks don't want to set up CHAP vs cases where they do (it's not like iSCSI is a secure protocol anyway).

@mikechristie
Copy link
Contributor Author

I messed up and can't seem to reopen this one.

How about this one

#45

I did not add any new gui parts for the target level. It just enables ACLs by default. We then have the chap action under the initiator and if that the user sets up chap then they have to use it for all clients. If they disable chap for all the clients and/or delete the clients with chap then we fall back to the default of ACL based.

I did not do the target level addition because it does not seem like a common case where someone sets up chap and then wants to remove it. Probably will just be a common case when you are playing around and you would probably just have a couple initiators and can just modify them individually. Also it seems like a waste if we ever add support to support a mix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants